Skip to content

MP1 feedback#1

Open
sarahwalters wants to merge 1 commit intomasterfrom
mp1-feedback
Open

MP1 feedback#1
sarahwalters wants to merge 1 commit intomasterfrom
mp1-feedback

Conversation

@sarahwalters
Copy link
Collaborator

Functionality: 5
Documentation: 5
Style: 5
Total: 5/5

This looks awesome! Your code is easy to read and well-structured, your documentation is clear, and the output is interesting to explore (the interactivity is a nice touch).

Two style comments:

  • Check out the if __name__ == "__main__" structure, which lets you run code on python relationship_map.py but not on import relationship_map. This isn't directly applicable to the scope of your project, but it's useful for larger-scale modularity.
  • Make sure you aren't importing things you don't need -- at this scale, from pattern.web import * won't slow your code down noticeably, but if you did something like that many times throughout a larger program, it would.

Also: it's polite to include a README with instructions for installing dependencies (I had most of them already, but I had to pip install selenium).

Look through the files changed to see more comments about specifics in your code. Feel free to respond to the comments inline -- more than happy to have a conversation.

Once you've read the comments, make a comment to the pull request letting us know that you've checked them out. Afterwards, close the PR.

REMEMBER NOT TO MERGE THESE PRs!

nshlapo pushed a commit that referenced this pull request Oct 13, 2015
Fixing word_frequency_analysis toolbox files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant